-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add await method about issue#7407 #7414
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@@ -433,7 +433,7 @@ export class Browser extends EventEmitter { | |||
url: 'about:blank', | |||
browserContextId: contextId || undefined, | |||
}); | |||
const target = this._targets.get(targetId); | |||
const target = await this._targets.get(targetId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether you've looked into the source code but this._targets
is a map of Target
instances. The assignment happens here:
puppeteer/src/common/Browser.ts
Lines 350 to 361 in 44b22bb
const target = new Target( | |
targetInfo, | |
context, | |
() => this._connection.createSession(targetInfo), | |
this._ignoreHTTPSErrors, | |
this._defaultViewport | |
); | |
assert( | |
!this._targets.has(event.targetInfo.targetId), | |
'Target should not exist before targetCreated' | |
); | |
this._targets.set(event.targetInfo.targetId, target); |
The target's constructor might be doing some async work, but you can't mark a constructor as async
and await the class instantiation (i. e. await new Target(...)
is not possible). The instance will be immediately returned and the constructor's async work happens afterwards in the background.
Did you actually try building puppeteer with this change to see whether this fixes the issue for you?
Edit: the only thing I could imagine is that adding the await
would defer this line to be executed later (i. e. pushes it to the end of the event loop). I just did a search and it seems like that's how it works according to this SO answer ("if you await things you will always defer the continuation to a microtask"). TIL 🤓
@mathiasbynens referring to #7351 (comment), is it possible that the await
deferring did actually have an effect?
03a4ca1
to
d4b17bd
Compare
I feel like this patch might not really be fixing the problem because the next line is already awaiting |
@zorori777 I'll close this for now as there hasn't been an update on this issue. Feel free to reopen if you think this is still relevant! |
Problem
issue
Analysis
When I updated this pakcage from 10.0.0 to 10.1.0, suddenly happened this error.
So I checked each differences in change log between 10.0.0 to 10.1.0.
ChangeLog
Reason
#7351
It shouldn't remove await method.
This pull request's approval comments say
There’s some async stuff happening in the constructor
, in the fact constructor call async method. so it need to use await function.According to error message, when synchronous processing finished, closed entire process at the same time.
Need to wait for asynchronous processing, so I add await method.
Fix
I add await method, removed it by #7351